Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Python Attribute Accessors and Cross-Library Compatibility for PkEncryption #15

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

TrevisGordan
Copy link

This PR adds missing Python attribute accessors and improves compatibility with python-olm by enabling cross-library encryption and decryption tests.

Details:

  • Adds #[pyo3(get)] annotations to the Message struct fields (ciphertext, mac, ephemeral_key), making these attributes accessible from Python.
  • Implements a Message constructor in Rust (via #[new]), allowing Python code to create Message instances directly.
  • Introduces tests to verify that attribute access works as intended.
  • Adds integration tests to ensure PkEncryption is compatible with python-olm. Specifically:
    • Encrypt with Olm and decrypt with Vodozemac.
    • Encrypt with Vodozemac and decrypt with Olm.

Next Steps:

To achieve seamless compatibility, the user will need to Base64-encode Vodozemac message attributes (ciphertext, mac, ephemeral_key) before passing them to Olm. For example:

vodo_encryption = PkEncryption.from_key(public_key)
vodo_message = vodo_encryption.encrypt(PLAINTEXT).encode())

vodo_mac = base64.b64encode(vodo_message.mac).decode('ascii')
vodo_ciphertext = base64.b64encode(vodo_message.ciphertext).decode('ascii')
vodo_ephemeral_key = base64.b64encode(vodo_message.ephemeral_key).decode('ascii')

A future enhancement could add a Rust helper function to handle this encoding automatically, such as:

(ciphertext, mac, ephemeral_key) = encryption.encrypt(json.dumps(media_body).encode()).to_base64()

Also unpad_base64_encode and pad_base64_decode functions.
This would streamline the process and reduce the amount of Python-side encoding logic required.

Adds python-olm Dependencie (at 3.2.16).
Adds PkEncryption (python-olm) compatibility tests.
Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this and sorry for the delay here. I left some small nits.

@@ -1,2 +1,3 @@
maturin
pytest>=4.0
python-olm==3.2.16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline.

ephemeral_key: Vec<u8>,
}

#[pymethods]
impl Message {
#[new]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some documentation for this method.

olm_msg = olm_encrypts.encrypt(CLEARTEXT)

# Convert the Olm message into a Vodo message structure.
def pad_base64_decode(b64_str: str) -> bytes:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with adding another constructor to the Message type, i.e. Message.from_base64(ciphertext, mac, key).

The same applies to the other direction: ciphertext, mac, key = message.to_base64().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes..., I’ve put this off for now and separated it out in issue #16. My Rust knowledge is currently quite limited, and Vec didn’t work out for me—especially given that the legacy olm.pk.PkEncryption uses a rather creative base64 encoding implementation.

But if you have a direction / idea, I'm happy to continue that part later on!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I remember this now. Doesn't https://docs.rs/vodozemac/latest/vodozemac/fn.base64_decode.html just work? As far as I know libolm just uses unpadded base64.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that’s the missing piece! I wasn’t aware of that function—thank you. Yes, for some (undocumented) reason it does behave that way; I initially only discovered it through trial and error.

With the compatible base64_decode/base64_encode functions, everything works perfectly now. I was able to implement a from_base64class method and a to_base64 method directly. Please take a look. I also removed that hacky b64 padding helper from the tests. Please double-check if everything is solid.

/// Create a new Message object from unpadded Base64-encoded components.
///
/// This function decodes the given Base64 strings and returns a `Message`
/// with the resulting byte vectors.
///
/// # Arguments
/// * `ciphertext` - Unpadded Base64-encoded ciphertext
/// * `mac` - Unpadded Base64-encoded message authentication code
/// * `ephemeral_key` - Unpadded Base64-encoded ephemeral key
#[classmethod]
fn from_base64(
_cls: &Bound<'_, PyType>,
ciphertext: &str,
mac: &str,
ephemeral_key: &str,
) -> Self {
let decoded_ciphertext =
vodozemac::base64_decode(ciphertext).expect("Failed to decode ciphertext");
let decoded_mac = vodozemac::base64_decode(mac).expect("Failed to decode mac");
let decoded_ephemeral_key =
vodozemac::base64_decode(ephemeral_key).expect("Failed to decode ephemeral_key");
Self {
ciphertext: decoded_ciphertext,
mac: decoded_mac,
ephemeral_key: decoded_ephemeral_key,
}
}
/// Convert the message components to unpadded Base64-encoded strings.
///
/// Returns a tuple of (ciphertext, mac, ephemeral_key) as unpadded Base64 strings.
fn to_base64<'py>(
&self,
py: Python<'py>,
) -> PyResult<(Bound<'py, PyString>, Bound<'py, PyString>, Bound<'py, PyString>)> {
let ciphertext_b64 = vodozemac::base64_encode(&self.ciphertext);
let mac_b64 = vodozemac::base64_encode(&self.mac);
let ephemeral_key_b64 = vodozemac::base64_encode(&self.ephemeral_key);
Ok((
ephemeral_key_b64.into_pyobject(py)?,
mac_b64.into_pyobject(py)?,
ciphertext_b64.into_pyobject(py)?,
))
}
}

Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some two minor things left to do, then I think we can merge.

ephemeral_key: &str,
) -> Self {
let decoded_ciphertext =
vodozemac::base64_decode(ciphertext).expect("Failed to decode ciphertext");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, we can't just expect() here. We should return a proper error. In other words this method needs to return a Result<Self, SomeError> type.

You can probably just extend the PK error type like such:

diff --git a/src/error.rs b/src/error.rs
index c91c256..b26716e 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -136,6 +136,8 @@ pub enum PkEncryptionError {
     InvalidKeySize(usize),
     #[error(transparent)]
     Decode(#[from] vodozemac::pk_encryption::Error),
+    #[error(transparent)]
+    Mac(#[from] vodozemac::Base64DecodeError),
 }
 
 pyo3::create_exception!(module, PkInvalidKeySizeException, pyo3::exceptions::PyValueError);
@@ -148,6 +150,7 @@ impl From<PkEncryptionError> for PyErr {
                 PkInvalidKeySizeException::new_err(e.to_string())
             }
             PkEncryptionError::Decode(_) => PkDecodeException::new_err(e.to_string()),
+            PkEncryptionError::Mac(_) => PkDecodeException::new_err(e.to_string()),
         }
     }
 }

/// Convert the message components to unpadded Base64-encoded strings.
///
/// Returns a tuple of (ciphertext, mac, ephemeral_key) as unpadded Base64 strings.
fn to_base64<'py>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this work just as well?

fn to_base64(&self) -> PyResult<(String, String, String)> {
    let ciphertext_b64 = vodozemac::base64_encode(&self.ciphertext);
    let mac_b64 = vodozemac::base64_encode(&self.mac);
    let ephemeral_key_b64 = vodozemac::base64_encode(&self.ephemeral_key);

    Ok((ephemeral_key_b64, mac_b64, ciphertext_b64))
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants